Skip to content

Remove the std::num::Times trait #11672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 30, 2014
Merged

Conversation

brendanzab
Copy link
Member

Times::times was always a second-class loop because it did not support the break and continue operations. Its playful appeal (which I liked) was then lost after do was disabled for closures. It's time to let this one go.

@flaper87
Copy link
Contributor

I liked the .times syntax 😢

@brendanzab
Copy link
Member Author

Let's discuss this further before r+-ing.

@brendanzab
Copy link
Member Author

Rebased with some fixes.

cc. @bstrie

@brendanzab
Copy link
Member Author

Another opeion is to have a Times iterator.

for () in 10.times() { ... }

@brendanzab
Copy link
Member Author

@flaper87: even without do? I thought the 'englishiness' of do x.times { ... } was the biggest appeal surrounding the method, hence why folks put up with its deficiencies as opposed to the language-supported iterators.

@liigo
Copy link
Contributor

liigo commented Jan 20, 2014

I like times and do,, don't remove them!

2014年1月20日 上午7:11于 "Brendan Zabarauskas" notifications@github.com写道:

@flaper87 https://github.com/FlaPer87: even without do? I thought the
'englishiness' of do x.times { ... } was the biggest appeal surrounding
the method, hence why folks put up with its deficiencies as opposed to the
language-supported iterators.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11672#issuecomment-32725530
.

@thestinger
Copy link
Contributor

@liigo: do for closures is already gone

@brendanzab
Copy link
Member Author

As @thestinger says, do notation only remains for procs, and it is up for debate whether even that should even be retained. I used to be a proponent of do, but I'm thinking it might be better to remove it entirely. spawn(proc() { ... }) isn't really that bad.

But anyway, the discussion of the fate of do for procs is beyond the scope of this PR. The important thing is that the syntax has been removed for closures, removing one of the nicest things about the Times::times method.

@adrientetar
Copy link
Contributor

Rust is not ruby; I don't see why we'd want to have literal syntax über alles.

Do is partly gone already and keeping it just for that makes no sense imo, I think it makes sense to cleanup the Times trait too; it not only lacks break and continue but (I think?) you can't make use of the iterator in the loop.
That being said, it would be cool to have the possibility to omit the first argument to range() for conciseness.

The do thing:
do i.times()
Range:
for _ in range(0, i)
What would be cool:
for _ in range(i)

@flaper87
Copy link
Contributor

FWIW, I meant to say that I liked the old n.times syntax, not that I'm voting to keeping it around the std. If for _ range(n, i) is better in every way than .times then I agree, we shouldn't keep it around.

As for the range(i) thing, I don't see what's wrong with just using range(b, i) which sticks more closely to the definition of mathematical intervals.

@adrientetar
Copy link
Contributor

@flaper87 range(i) keeps the "do it i times" idiom, but with the power of range iterators. This is a possible pattern in Python.

@brendanzab
Copy link
Member Author

range(i) is kinda odd. It's not a range, just a single value. It's also not possble to have both in Rust - we don't have function overloading.

@flaper87
Copy link
Contributor

Exactly my point. I know python has support for it. I just don't think we
should have it in rust. I don't personally like the implicit semantics of
it.

@adrientetar
Copy link
Contributor

Means "range with i elements" to me, doesn't seem that odd.

@brson
Copy link
Contributor

brson commented Jan 21, 2014

+1

@alexcrichton
Copy link
Member

r=me with a rebase, sounds like we've got enough people on board.

@brendanzab
Copy link
Member Author

Ok, I'll try to update this.

@brendanzab
Copy link
Member Author

This seems to have already been done since I made this commit. Good work guys!

@brendanzab brendanzab closed this Jan 29, 2014
@brendanzab brendanzab reopened this Jan 29, 2014
@brendanzab
Copy link
Member Author

Oh huh. I think I made a mistake. Still there. Ok, I will still work on rebasing. Sorry!

`Times::times` was always a second-class loop because it did not support the `break` and `continue` operations. Its playful appeal was then lost after `do` was disabled for closures. It's time to let this one go.
@brendanzab
Copy link
Member Author

Ok, rebased! r?

bors added a commit that referenced this pull request Jan 30, 2014
`Times::times` was always a second-class loop because it did not support the `break` and `continue` operations. Its playful appeal (which I liked) was then lost after `do` was disabled for closures. It's time to let this one go.
@bors bors merged commit 729060d into rust-lang:master Jan 30, 2014
@brendanzab brendanzab deleted the remove-times branch January 30, 2014 05:29
@bstrie
Copy link
Contributor

bstrie commented Feb 2, 2014

Good riddance! :)

flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 21, 2023
[`unnecessary_lazy_eval`]: reduce applicability if closure has return type annotation

Fixes rust-lang#11672

We already check if closure parameters don't have type annotations and reduce the applicability to `MaybeIncorrect` if they do, since those help type inference and removing them breaks code. We didn't do this for return type annotations however. This PR adds it. This doesn't change it to produce a fix that will compile, but it will prevent rustfix from auto-applying it.

(In general I'm not sure if we can suggest a fix that will compile. In this specific example, it might be possible to suggest `&[] as &[u8]`, but as-casts won't always work, e.g. `Default::default() as &[u8]` is a compile error, so just reducing applicability should be a safe fix in any case for now)

changelog: [`unnecessary_lazy_eval`]: reduce applicability to `MaybeIncorrect` if closure has return type annotation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants